Add -mbmi2 flag when -mavx2 is used in Makefiles
#992
Conversation
The `-mbmi2` flag can be used in conjunction with `-mavx2`. This was done in some places but not consistently.
sergeisakov
left a comment
sergeisakov
left a comment
There was a problem hiding this comment.
LGTM. However, there are some old CPUs that support avx2 and don't support bmi2. Perhaps we can discontinue support for these CPUs given their limited usage.
It's unclear that adding test logic to detect bmi2 would be the right thing to do in this particular Makefile. For the time being, I'm reverting this back to how it was before this PR.
Per discussion in review, it is possible for some older processors to support AVX2 but not BMI2, so those should be tested separately. Also, this slightly reorganizes the flags code to put things in alphabetical order, and also adds the flags to the `print-vars` target.
This adds a check for whether the host supports BMI2, and only adds the flag if so.
github-actions
bot
added
size: S
The bazel build files need to be reworked. Let's make this PR be about the cmake & make files.
Thank you again for that tip @sergeisakov. I reworked the makefile & cmake changes so that now it tests if BMI2 is actually supported before it adds the flag. Could you take another look at the new version of this PR when you get a chance? |
pybind_interface/avx2/CMakeLists.txt
Outdated
| add_compile_options($<$<NOT:$<CONFIG:Debug>>:/O2>) | ||
| else() | ||
| include(CheckCCompilerFlag) | ||
| check_c_compiler_flag("-mbmi2" COMPILER_HAS_BMI2) |
There was a problem hiding this comment.
That just checks if the compiler accepts this flag. BMI2 might still not be supported by CPU. I think this is okay for now. Perhaps we should have two AVX2 versions (with BMI2 and without) and detect one at runtime as we now detect SSE/AVX2/AVX512. We can add the second version later.
There was a problem hiding this comment.
That just checks if the compiler accepts this flag. BMI2 might still not be supported by CPU. I think this is okay for now. Perhaps we should have two AVX2 versions (with BMI2 and without) and detect one at runtime as we now detect SSE/AVX2/AVX512. We can add the second version later.
Yeah, I confess I used the compiler flags because I couldn't find a cmake construct to do it properly. Of course you're right, it's incorrect and lazy.
So I searched for a better way. I found some alternatives, but they are more involved and IMHO better left to a separate PR. The simplest solution for this PR seems to be to use grep as is done in tests/Makefile. The latest push to this PR has the change to the CMakeLists.txt to do it that way instead of checking compiler flags.
@sergeisakov sorry to ask you to take yet another look …
As Sergei commented in the PR review, using the cmake test for compiler options is not the right way to test for BMI2 support in the CPU. Change it to use the same approach used in the Makefile in the tests/ directory.
2 participants
It looks like it's possible to use the
-mbmi2flag when-mavx2is used, based on documentation and on testing locally. This was done in some Makefiles but not consistently. This PR adds the flag to Makefiles where it was missing, plus also in one BUILD file.